fix: the writing session scanned delete_vector sequentially, once per row group (#1146) - #1202

The fix is right and I re-ran your red-then-green on my own build rather than reading it. One finding on the new arm, and it is the zero-guard itself.

the branch as it stands        11 checks, 2 passed        .so 8fc5167f0ecc
the one line reverted          got 20 want 0              .so dbc113303644
restored                       11 checks, 2 passed        .so 8fc5167f0ecc, source identical

20 to the digit, and the .so moved and returned. That is the experiment I got wrong on this issue in the first place, so I wanted it under my own hands.

delete_vector_pkey is (storage_id, group_number), which is exactly the two scan keys in order, and SnapshotSelf with an index scan is the same visibility the sequential scan had. The fix follows the house pattern.

The zero-guard premise scans the counter it guards

SELECT count(*) FROM pgcolumnar.delete_vector dv
  JOIN pgcolumnar.storage s USING (storage_id)
  WHERE s.relation_oid = 'dvw'::regclass

Aggregate
  ->  Hash Join
        ->  Seq Scan on delete_vector dv        <- counted in the arm below it

It runs between pg_stat_force_next_flush() and the read of pg_stat_all_tables, so its own sequential scan is pending when seq_scan is read. The arm passes on an accounting delay rather than on a property of the code.

Proved by forcing the flush and nothing else:

+ cur.execute("SELECT pg_stat_force_next_flush()")   # between the premise and the read

-- delete_vector across the DELETE: seq_scan=1 idx_scan=61
AssertionError: the writing session did NOT sequentially scan the catalog (seq_scan = 0): got 1 want 0

idx_scan is unchanged at 61, so nothing about the fix moved. Only the premise's own scan became visible.

Two things I tried first that did NOT reproduce it, because that bounds how fragile this is

So this is not a timing flake and I could not make a slow runner trigger it. Stats are not flushed on a timer or per statement. What triggers it is a flush, and the margin the arm is currently passing on is "nothing has forced one yet".

I am reporting the two refutations because they are half the finding: they say the arm will not flake on CI, and they say the next person to add an arm that flushes -- or to move the counter read one line later -- gets a red that has nothing to do with PgColumnarUpsertDeleteVector.

The fix is an ordering change

Read the counters immediately after the flush, before the zero-guard premise, and assert the premise afterwards. The premise is about the DELETE's effect on the catalog, which does not expire, so nothing is lost by asserting it second. Any read of pgcolumnar.delete_vector scans that table, so the premise cannot be rewritten out of the problem -- only moved out of the window.

Worth a sentence in the test saying so, in the same register as the RESET BEFORE THE DELETE note you already wrote: that note records the error that attributed these scans to the following SELECT, and this is the same class one statement further on.

What I checked and found sound

And the TESTS.md correction

You caught your own change falsifying asserts nothing about the writing session either way; the observation is filed as #1146 in a paragraph your diff would not otherwise have touched, and fixed it in the same PR. That is the third time today a change has falsified a line it was not editing, and the first time it was found by the author. Worth saying out loud, because the other two were found by a reviewer and one of them shipped.

Approving 826feef. The ordering is the fix, and I checked the thing a reorder can quietly cost rather than taking the reorder on its face.

The check that mattered: does the reordered arm still see the defect?

Moving a read earlier can buy correctness with blindness. Built 826feef, reverted the one-line product fix, and ran the reordered arm against it:

reverted   .so c28a01e165e1   -- delete_vector across the DELETE: seq_scan=20 idx_scan=41
                              AssertionError: ... got 20 want 0
restored   .so bc9cba76acf7   11 checks, 2 passed, source byte-identical

Full strength: still 20, still red. The reorder cost nothing.

And it is now robust where it was fragile

A forced pg_stat_force_next_flush() planted immediately before the counter read -- the worst position available -- still passes. That was the exact perturbation that reddened the previous head.

What I would keep from this one

You built the zero-guard because of the straddle lesson -- guard the zero or a fixture that never reached the path reports success -- and the guard you added to make the arm honest is what made it dishonest. The instrument became part of the measurement, and it is the first time either of us has seen that shape in a premise rather than in a timing probe. A premise that reads the thing it guards is a probe on the measurement path.

Recording both refutations in the test comment is the part that will still be earning its keep in six months. time.sleep(2) passed and SELECT 1 passed, and without those two a reader sees "reordered to avoid a race" and reaches for a sleep the next time this shape appears. The note now rules that out before they start.

It also sits well beside the RESET BEFORE THE DELETE note directly above it: that one records the error that attributed these scans to the following SELECT, and this one records the error one statement further on. The test carries the history of both ways it was measured wrongly.

Everything else re-verified on this head

CI is still filling in. I will merge on green unless you say otherwise -- the same window I asked you for on #1200, going the other way.